agones-{extensions,allocator}: Pause after cancelling context #3843

zmerlynn · 2024-05-30T17:19:04Z

After #3839 went in, we noticed the flakes in TestAllocatorAfterDeleteReplica disappear, but TestGameServerCreationRightAfterDeletingOneExtensionsPod remained. I looked more closely at this and I developed a theory: My guess is that kube-apiserver connections to webhooks are actually "sticky" using http(s) keepalives. If the TCP connection never closes, we'd see the behavior we do, which is the EOF from kube-apiserver going to write on a dead socket.

I looked at how to close sockets on the server side to "finish" the drain, but realized that we actually do, by cancelling the context. The thing is that we cancel the context and immediately exit, but it leaves no time for anything to shut down.

Of course, then I looked more into this and ... I'm not sure why this is helping. But I was able to reproduce the e2e flake locally, and with this small change I'm able to see it go away, so 🤷 .

Reverts #3839

This reverts commit 68b04ee.

After googleforgames#3839 went in, we noticed the flakes in TestAllocatorAfterDeleteReplica disappear, but TestGameServerCreationRightAfterDeletingOneExtensionsPod remained. I looked more closely at this and I developed a theory: My guess is that `kube-apiserver` connections to webhooks are actually "sticky" using http(s) keepalives. If the TCP connection never closes, we'd see the behavior we do, which is the `EOF` from kube-apiserver going to write on a dead socket. I looked at how to close sockets on the server side to "finish" the drain, but realized that we actually do, by cancelling the context. The thing is that we cancel the context and immediately exit, but it leaves no time for anything to shut down. I tested this and it seems to drive away the flake, without adding in the extra delay. Reverts googleforgames#3839

agones-bot · 2024-05-30T19:21:03Z

Build Succeeded 👏

Build Id: 8679ca3f-4c75-46fe-b0bd-9a1408b22e87

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://6dda265-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3843/head:pr_3843 && git checkout pr_3843
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.41.0-dev-6dda265-amd64

markmandel

I'm down to give it a shot 😁

agones-bot · 2024-05-30T23:04:04Z

Build Succeeded 👏

Build Id: 06c30836-c078-47ed-9094-84555411c0f7

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://90dcbf2-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3843/head:pr_3843 && git checkout pr_3843
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.41.0-dev-90dcbf2-amd64

I ran the HA tests on googleforgames#3843 overnight in a loop and still noticed a very low grade flake in the allocator, so I decided to go ahead and clean up the issues I noticed while working on the previous PR: * adds an `httpserver` utility package to handle the `Run` function that controller/extensions use. Make that context aware using the same method as https.Run: https://github.com/googleforgames/agones/blob/dfa414e5e4da37798833bbf8c33919acb5f3c2ea/pkg/util/https/server.go#L127-L130 (note that I think this ^ is why the extensions flake disappeared after I added a pause). * also plumbs context-awareness through the allocator run{Mux,REST,GRPC} functions, which is I suspect hy we're seeing a low-grade flake in allocator still. (I think the delay I added previously has sufficient to drive it off, too, but this PR should be more better.)

zmerlynn added 2 commits May 29, 2024 19:28

Revert "agones-{extensions,allocator}: Be more defensive about draining"

faf02b3

This reverts commit 68b04ee.

zmerlynn assigned markmandel May 30, 2024

github-actions bot added kind/other size/S labels May 30, 2024

markmandel approved these changes May 30, 2024

View reviewed changes

markmandel enabled auto-merge (squash) May 30, 2024 22:03

Merge branch 'main' into wait-after-cancel

90dcbf2

markmandel merged commit 80dd2ea into googleforgames:main May 30, 2024
4 checks passed

zmerlynn mentioned this pull request May 31, 2024

agones-{extensions,allocator}: Make servers context aware #3845

Merged

Kalaiselvi84 added kind/bug These are bugs. and removed kind/other labels Jun 4, 2024

zmerlynn mentioned this pull request Jun 4, 2024

Fix connection-draining flakes in TestAllocatorAfterDeleteReplica / /TestGameServerCreationRightAfterDeletingOneExtensionsPod #3853

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agones-{extensions,allocator}: Pause after cancelling context #3843

agones-{extensions,allocator}: Pause after cancelling context #3843

zmerlynn commented May 30, 2024

agones-bot commented May 30, 2024

markmandel left a comment

agones-bot commented May 30, 2024

agones-{extensions,allocator}: Pause after cancelling context #3843

agones-{extensions,allocator}: Pause after cancelling context #3843

Conversation

zmerlynn commented May 30, 2024

agones-bot commented May 30, 2024

markmandel left a comment

Choose a reason for hiding this comment

agones-bot commented May 30, 2024